Repository navigation
feat(mcp): limit, having and a default order for clickstack_table - #3558
Conversation
clickstack_table had no limit or having, and without orderBy a grouped result came back in arbitrary order, so trimming kept arbitrary groups. Add optional limit and having (passed into the chart config through runConfigTile options, MCP only), and order grouped tables by the first select item, descending, when no orderBy is given. Dashboard tiles and the app are unchanged. Refs ClickHouse#3555.
|
@bcanvural is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: e778e2a The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * built from CTEs, and `increase` already orders its top-N groups. | ||
| */ | ||
| /** @internal Exported for testing only. */ | ||
| export function defaultOrderBy( |
There was a problem hiding this comment.
🔵 minor — defaultOrderBy re-implements the default ordering in convertToCategoricalChartConfig, without its tiebreak
convertToCategoricalChartConfig (packages/common-utils/src/core/utils.ts:1070-1097) already adds a default 'first select item DESC' order. It quotes the alias, or sets it to 'Value' when there is none, and adds the groupBy ASC as a stable tiebreak. defaultOrderBy does the same job a second way (positional 1, no tiebreak). So with limit, groups that tie on the first value can come back differently on each call. Pull the shared logic into one common-utils helper and use it for both, or at least add the groupBy ASC tiebreak here so limited results are deterministic.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| 'Reference a select item by its alias (e.g. "Errors > 10") or repeat the aggregate.', | ||
| ), | ||
| orderBy: orderBySchema.describe( | ||
| 'Sort results by this column. Defaults to the first select item, descending, ' + |
There was a problem hiding this comment.
🔵 minor — orderBy description promises a default order that metric sources and pie/bar shapes don't get
The schema text says results default to the first select item, descending, whenever groupBy is set. But defaultOrderBy returns undefined for metric selects and for every shape except 'table'. Pie/bar only get a default order through convertToCategoricalChartConfig, and only when a limit is set. An agent querying a grouped metric table will expect ordered, top-N-safe results and get arbitrary order after trimming. Narrow the description (e.g. 'for non-metric sources with shape "table"'), or extend the default to cover those cases.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const tableToolOverrides = isSearch | ||
| ? {} | ||
| : { | ||
| ...(options?.having |
There was a problem hiding this comment.
🔵 minor — A whitespace-only having passes the truthiness check
options?.having ? … lets a value like ' ' through, so it gets set on the chart config with havingLanguage 'sql'. The having schema in table.ts has no .trim() or .min(1). Add .trim() to the schema (or check options.having.trim()) so an empty condition is dropped rather than depending on the renderer's isNonEmptyWhereExpr guard.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review✅ No issues found. |
Deep Review✅ No critical issues found. No P0/P1: 🟡 P2 -- recommended
🔵 P3 nitpicks (5)
Reviewers (9): correctness, adversarial, security, api-contract, testing, kieran-typescript, maintainability, previous-comments, project-standards. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
…By/having descriptions, trim having Add the groupBy columns as an ASC tiebreak after the default first-item DESC, as convertToCategoricalChartConfig does, so a limit keeps the same groups. Say the default applies to shape "table" on non-metric sources, recommend referencing select aliases in having, and trim having so a blank value is dropped.
|
Thanks for the reviews. Addressed in e3f65a1 (all minor):
Not changed: a shared helper with Unit (488) and the query tool integration tests (95) pass. |
| if (selectItems.length === 0 || selectItems.some(s => s.metricType)) { | ||
| return undefined; | ||
| } | ||
| const alias = selectItems[0].alias?.trim(); |
There was a problem hiding this comment.
🔵 minor — Default ORDER BY trims the alias, but SELECT keeps the untrimmed alias, so the ORDER BY names a column that doesn't exist
mcpSelectItemSchema.alias in schemas.ts isn't trimmed, and renderSelectList emits it verbatim as AS " Errors" (packages/common-utils/src/core/renderChartConfig.ts:814-815). defaultOrderBy trims it first and emits Errors DESC, which ClickHouse can't resolve. Before this PR, a grouped call with that alias and no orderBy worked; now it fails. Fix: build the ORDER BY from the raw alias, i.e. quoteAliasForOrderBy(selectItems[0].alias), and only use .trim() to check that the alias isn't empty. Alternatively, trim alias in the schema so SELECT and ORDER BY see the same value.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
…est limit/having on pie and bar; cap limit Quote the first select alias untrimmed so ORDER BY names the rendered column. Add integration tests for limit and having on the pie/bar shapes (through convertToCategoricalChartConfig) and for a quoted alias with a space. Cap limit at 10,000, say metric sources need an explicit orderBy with limit, and type defaultOrderBy's displayType as the shape union.
|
Round 2, in the latest commit:
Not changed: moving the Unit (489) and query tool integration tests (100) pass. That's two rounds, so per AGENTS.md I'll stop changing things here. @brandon-pereira, ready for a look when you have time. |
…ordered reductions, trim notes (#3554) Refs #3542 Follows the proposal in the maintainer comment on #3542 (the three points, in order). Kept to `trimToolResponse`; the table tool's `having` / `limit` will go to a separate issue and PR as asked. ## What 1. **Shorten instead of replace** (`packages/api/src/utils/trimToolResponse.ts`). A string over its budget is cut and gets a `…(+N chars)` suffix (e.g. `…(+6,900 chars)`), instead of becoming `{"__hdx_trimmed":true,"originalSize":N}`. This applies to every tool through the fallback. In arrays, items within an equal share stay whole and the large items split what's left, so shortened items add up to the array's budget (with replacement markers that didn't matter; with shortened strings it does). 2. **Trim notes.** `TrimResult` gains `notes: string[]`: the size before/after and limit, each reduction that changed something (e.g. `patterns[].samples: kept the first 1 (60 elements removed from 30 items)`), arrays cut by path (`patterns: cut from 30 to 10 items`), and how many values were shortened. Every caller this PR changes that already adds a `note` when trimmed now also returns them as `trimNotes`; `clickstack_describe_table` is not changed here. 3. **Ordered reductions.** `trimToolResponse(data, maxSize, { reductions })` with `{ path, keep }`, `{ path, maxChars }` and `{ path, drop: true }`, using the path syntax from the proposal (`patterns[].samples`, `patterns[].samples[]`). It works as described: - a response that fits is returned unchanged (same object); - otherwise the reductions run in order on a copy, each on every match, and the whole response is measured after each step; it stops as soon as it fits; - only then the existing per-key split and array cuts run. `clickstack_event_patterns` uses the list from the proposal (samples keep 3, keep 1, samples[] maxChars 2000, pattern maxChars 2000, trend drop), plus a last step: `patterns[].whereSnippet` is dropped, never shortened, because a cut drill-down predicate is invalid SQL. Since it's dropped before the fallback runs, the fallback (the only part that shortens strings) never sees one; the `note` says when snippets were removed. `clickstack_emerging_signals` shortens `emerging[].pattern` / `disappeared[].pattern` to 2,000 chars first. 4. **Smaller fixes from review.** `shortenString` never makes a value longer and doesn't split surrogate pairs; array item budgets count the separators; array cuts are noted once per path and size markers are noted; note paths match the response (`result.…`); `search` with `denoise` keeps the first trim's notes; the `clickstack_sql` batch appends its row cut to the per-query notes. ## Effect An `event_patterns`-shaped result with 30 patterns, the top 3 being ~4.5 KB stack traces with 5 samples each (101,450 chars, limit 50,000): | | Patterns returned | Top pattern | Size | |---|---|---|---| | `main` | 10 | `{"__hdx_trimmed":true,"originalSize":4736}` | 3,848 | | Fallback only (this PR, no reductions) | 10 | shortened, with `…(+N chars)` | 8,282 | | With the event_patterns reductions | **30** | complete | 41,082 (stopped after "samples keep 1") | ## Tests `packages/api/src/utils/__tests__/trimToolResponse.test.ts` (12 new): - suffix format; shortened (not replaced) strings in the fallback within the budget; notes for size, shortened values and array cuts; no notes when nothing is trimmed; - each reduction kind on every match, with its note; stop-early (later steps don't run once it fits); unchanged when it fits; input not mutated; fallback after reductions stays within the budget; - the 30-pattern case above: all 30 kept, no `__hdx_trimmed`. - JSON-escape expansion (quotes, control chars, non-ASCII, `…`) stays within the budget; separators-only arrays; no-growth and surrogate cases; grouped notes; `drop` on array elements. `packages/api/src/mcp/__tests__/queryTool.int.test.ts`: the real `event_patterns` tool with 30 large patterns keeps all 30, drops (never cuts) `whereSnippet`, and returns `trimNotes`. `sqlToolBatch.test.ts`: batch cuts replace `trimNotes`. `npx jest src/mcp src/utils --testPathIgnorePatterns '\.int\.test\.ts$'`: 932 passed; `queryTool` + `emergingSignalsTool` int tests: 117 passed. tsc, eslint (no new warnings) and prettier clean on the changed files. ## Not included - `having` / `limit` / default order for `clickstack_table`: separate issue and PR (#3555, #3558), as asked.
Closes #3555
What
MCP surface only; dashboard tiles and the app don't change.
limit(optional, positive integer): SQLLIMIT, applied after ordering. No default, so a call without it returns the same rows as today.having(optional): post-aggregation SQLHAVING. It can use a select item's alias ("Errors > 10") or repeat the aggregate ("count() > 100").groupByand noorderBy, atableresult is ordered by the first select item, descending. Today the order is whatever ClickHouse returns, so response trimming keeps arbitrary groups. An explicitorderByis used exactly as before.How:
defaultOrderByinpackages/api/src/mcp/tools/query/table.ts), next toresolveOrderBy, not inrenderChartConfig. It uses the first item's alias when it has one, otherwise its position (ORDER BY 1 DESC): unaliased select items have no name to refer to, and the renderedSELECTlists the select items before thegroupBycolumns. It is skipped for metric selects (their SQL is built from CTEs, andincreasealready orders its top-N groups) and for thenumber/pie/barshapes.havingandlimitreach the chart config through two new optionalrunConfigTileoptions, set only byclickstack_table. The table tile schema has nolimit, and pie/bar tiles have nohaving, so passing them through the tile would drop them for some shapes; the options apply to every shape and leave the tile schemas alone. Other callers ofrunConfigTiledon't pass them, so nothing changes for them.Tests
query.test.ts:defaultOrderBy(position vs alias, quoted alias, no default withoutgroupBy, for other shapes, or for metric selects).queryTool.int.test.ts, against ClickHouse with 3 services of 1/5/3 rows: default order with an alias and by position, explicitorderByunchanged,limitafter ordering,havingon an alias.queryTool.int.test.ts: 95 pass.tsc --noEmit, eslint (no new warnings) and prettier are clean.Not included: unknown input keys are still stripped silently; that's #3543.